Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

feat(string): add binary and octal random string generation #1710

Merged
merged 7 commits into from
Jan 5, 2023
Merged

feat(string): add binary and octal random string generation #1710

merged 7 commits into from
Jan 5, 2023

Conversation

pladreyt
Copy link
Contributor

@pladreyt pladreyt commented Jan 2, 2023 •
edited
Loading

This PR implements binary and octal random string generation.

Issue: #184

PR that implements this feature in number module: #1708

@pladreyt pladreyt requested a review from a team as a code owner January 2, 2023 18:23
@pladreyt pladreyt mentioned this pull request Jan 2, 2023
Merged
@import-brain import-brain added c: feature Request for new feature p: 1-normal Nothing urgent m: string Something is referring to the string module labels Jan 2, 2023
ST-DDT
ST-DDT reviewed Jan 2, 2023
View reviewed changes
Copy link
Member

@ST-DDT ST-DDT left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Especially octal and binary seem to have a multitude of common prefixes, should we use '' as the default prefix for all of them?

Only one of our methods uses a non-empty prefix (etheriumAddress), all other 5 methods use ''.

src/modules/string/index.ts Outdated Show resolved Hide resolved
src/modules/string/index.ts Outdated Show resolved Hide resolved
@codecov
Copy link

codecov bot commented Jan 2, 2023 •
edited
Loading

Codecov Report

Merging #1710 (6ece169) into next (d3229fc) will increase coverage by 0.00%.
The diff coverage is 100.00%.

Additional details and impacted files 
@@           Coverage Diff           @@
##             next    #1710   +/-   ##
=======================================
  Coverage   99.63%   99.63%           
=======================================
  Files        2249     2249           
  Lines      240482   240573   +91     
  Branches     1083     1095   +12     
=======================================
+ Hits       239607   239698   +91     
  Misses        854      854           
  Partials       21       21
Impacted Files Coverage Δ
src/modules/number/index.ts 100.00% <100.00%> (ø)
src/modules/string/index.ts 100.00% <100.00%> (ø)

@ST-DDT ST-DDT added the s: needs decision Needs team/maintainer decision label Jan 2, 2023
@pladreyt
Copy link
Contributor Author

pladreyt commented Jan 2, 2023

Especially octal and binary seem to have a multitude of common prefixes, should we use '' as the default prefix for all of them?

Only one of our methods uses a non-empty prefix (etheriumAddress), all other 5 methods use ''.

Also the hexadecimal method uses a non-empty prefix, I was using it as a base. Was keeping the prefix for consistency with that method, but I can set it to ''

@ST-DDT
Copy link
Member

ST-DDT commented Jan 3, 2023

Please add @see to and from the number module.

@pladreyt
Copy link
Contributor Author

pladreyt commented Jan 3, 2023

Please add @see to and from the number module.

Sure!

@pladreyt pladreyt force-pushed the feature-184-add-binary-and-octal-random-string-generation branch 3 times, most recently from c16ef52 to f0836be Compare January 3, 2023 17:59
@pladreyt pladreyt force-pushed the feature-184-add-binary-and-octal-random-string-generation branch from f0836be to 28d4c8a Compare January 3, 2023 18:00
@pladreyt pladreyt requested review from ST-DDT and Shinigami92 and removed request for ST-DDT and Shinigami92 January 3, 2023 18:21
ST-DDT
ST-DDT reviewed Jan 3, 2023
View reviewed changes
Copy link
Member

@ST-DDT ST-DDT left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Two minor changes, then we are good to go from my side.

src/modules/string/index.ts Outdated Show resolved Hide resolved
src/modules/string/index.ts Outdated Show resolved Hide resolved
@ST-DDT ST-DDT removed the s: needs decision Needs team/maintainer decision label Jan 3, 2023
@ST-DDT ST-DDT added the s: accepted Accepted feature / Confirmed bug label Jan 3, 2023
ST-DDT
ST-DDT approved these changes Jan 4, 2023
View reviewed changes
Copy link
Member

@ST-DDT ST-DDT left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for your contribution.

@ST-DDT ST-DDT requested review from Shinigami92 and a team January 4, 2023 17:37
@ST-DDT ST-DDT merged commit 80e0a96 into faker-js:next Jan 5, 2023
@ST-DDT ST-DDT mentioned this pull request Jan 7, 2023
Closed
@xDivisionByZerox xDivisionByZerox mentioned this pull request Jan 21, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Shinigami92 Shinigami92 Shinigami92 approved these changes

@ST-DDT ST-DDT ST-DDT approved these changes

Assignees

@pladreyt pladreyt

Labels
c: feature Request for new feature m: string Something is referring to the string module p: 1-normal Nothing urgent s: accepted Accepted feature / Confirmed bug
Projects
No open projects
Faker Roadmap
Status: Done
Milestone
v8.0 - Module Re-Shuffling
Development

Successfully merging this pull request may close these issues.
4 participants
@pladreyt @ST-DDT @Shinigami92 @import-brain